Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[TS migration] Setup typescript in expensify-common & Migrate CONST, fastMerge, Device, Logger to Typescript #699

Merged
merged 28 commits into from
May 27, 2024
Merged

[TS migration] Setup typescript in expensify-common & Migrate CONST, fastMerge, Device, Logger to Typescript #699

merged 28 commits into from
May 27, 2024

Conversation

blazejkustra
Copy link
Contributor

@blazejkustra blazejkustra commented May 17, 2024 •
edited
Loading

Going forward expensify-common code can be written in Typescript, we have to take into consideration transpiling step now.

TS code will live in lib catalog and transpiled code will end up in dist catalog. The lib catalog won’t be published to npm; instead, we’ll only publish the dist catalog.

⚠️ This means that from now on expensify-common imports will look like this:

// Before:
import ExpensiMark from 'expensify-common/lib/ExpensiMark';

// After:
import {ExpensiMark, Str} from 'expensify-common';
// or
import ExpensiMark from 'expensify-common/dist/ExpensiMark';

Here is a similar PR that prepared Onyx for a similar migration.

Fixed Issues

$ Expensify/App#42245

@blazejkustra blazejkustra changed the title [TS migration] Setup typescript in expensify-common [TS migration] Setup typescript in expensify-common & Migrate CONST, fastMerge, Device, Logger to Typescript May 17, 2024
jnowakow
jnowakow approved these changes May 20, 2024
View reviewed changes
Copy link

@jnowakow jnowakow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

lib/ReportHistoryStore.jsx Show resolved Hide resolved
@blazejkustra blazejkustra marked this pull request as ready for review May 20, 2024 14:35
@blazejkustra blazejkustra requested a review from a team as a code owner May 20, 2024 14:35
@melvin-bot melvin-bot bot requested review from carlosmiceli and removed request for a team May 20, 2024 14:35
fabioh8010
fabioh8010 reviewed May 20, 2024
View reviewed changes
lib/Logger.ts Outdated Show resolved Hide resolved
lib/Templates.jsx Show resolved Hide resolved
@carlosmiceli carlosmiceli requested a review from tgolen May 22, 2024 00:02
@carlosmiceli
Copy link
Contributor

@tgolen I thought I'd bring you in to review this one, since I may not be the best suited to confirm that everything is as it should... I haven't seen anything wrong in the code, but I think a second more veteran opinion is worth it here.

@tgolen
Copy link
Contributor

tgolen commented May 22, 2024

I'm really sorry. I was not able to review this today due to being heads-down getting comp review ready. I will be OOO for the next 1.5 week, so I will assign someone else to review.

@tgolen tgolen requested review from roryabraham and removed request for tgolen May 22, 2024 22:41
roryabraham
roryabraham requested changes May 23, 2024
View reviewed changes
Copy link
Contributor

@roryabraham roryabraham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, what is the plan for publishing this? Right now, expensify-common is consumed via a git hash in E/App's package.json

That could work if we commit dist, but I see you've addd it to the .gitignore. So what's the plan? If we need to publish this to npm can you copy the workflow from react-native-onyx?

.eslintrc.js Show resolved Hide resolved
lib/Logger.ts Outdated Show resolved Hide resolved
lib/ExpenseRule.jsx Show resolved Hide resolved
@carlosmiceli carlosmiceli removed their request for review May 23, 2024 18:30
@blazejkustra
Copy link
Contributor Author

Also, what is the plan for publishing this? Right now, expensify-common is consumed via a git hash in E/App's package.json

That could work if we commit dist, but I see you've addd it to the .gitignore. So what's the plan? If we need to publish this to npm can you copy the workflow from react-native-onyx?

I think copying the workflow from react-native-onyx makes the most sense, so that we consume the package from npm.

@blazejkustra
Copy link
Contributor Author

blazejkustra commented May 27, 2024 •
edited
Loading

@roryabraham I copied over the publish workflow from Onyx, I think the last thing we need is to add the missing secrets. Would you be able to copy it over from Onyx?

  • OS_BOTIFY_COMMIT_TOKEN
  • LARGE_SECRET_PASSPHRASE
  • NPM_TOKEN

FYI: There is expensify-common in npm's registry already.

If you think this PR is too big, I can move publish workflow and related changes to a separate PR.

@blazejkustra
Copy link
Contributor Author

Also, please have another look at the code, I changed it slightly! 🚀

@blazejkustra blazejkustra requested a review from roryabraham May 27, 2024 10:43
fabioh8010
fabioh8010 approved these changes May 27, 2024
View reviewed changes
.eslintrc.js Show resolved Hide resolved
roryabraham
roryabraham approved these changes May 27, 2024
View reviewed changes
Copy link
Contributor

@roryabraham roryabraham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but let's get those secrets added before we merge

@roryabraham
Copy link
Contributor

Created internal ticket to add those GitHub secrets

@roryabraham roryabraham merged commit c3d4ccb into Expensify:main May 27, 2024
6 checks passed
@roryabraham
Copy link
Contributor

Let's see if publishing works as expected... https://github.com/Expensify/expensify-common/actions/runs/9261388961

@github-actions GitHub Actions
Copy link

🚀Published to npm in v2.0.1

@roryabraham
Copy link
Contributor

Looks like that worked (after a few more minor adjustments) 🎉

@blazejkustra
Copy link
Contributor Author

Awesome! Thanks @roryabraham for the help on this one 🙌

@blazejkustra blazejkustra mentioned this pull request May 28, 2024
Merged
@tsa321 tsa321 mentioned this pull request May 29, 2024
Merged
50 tasks
@blazejkustra blazejkustra mentioned this pull request May 29, 2024
Merged
49 tasks
@blazejkustra blazejkustra mentioned this pull request Aug 26, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jnowakow jnowakow jnowakow approved these changes

@roryabraham roryabraham roryabraham approved these changes

@kubabutkiewicz kubabutkiewicz kubabutkiewicz approved these changes

@fabioh8010 fabioh8010 fabioh8010 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@blazejkustra @carlosmiceli @tgolen @roryabraham @fabioh8010 @kubabutkiewicz @jnowakow